plugins/operators/__init__.py (5)

167-167: Remove unused variable sample_ids

The variable sample_ids is assigned but never used. Remove the assignment if it is unnecessary.

Apply this diff to fix the issue:

-    sample_ids = ctx.dataset.add_collection(samples, new_ids=True)
+    ctx.dataset.add_collection(samples, new_ids=True)

🧰 Tools

🪛 Ruff (0.8.2)

167-167: Local variable sample_ids is assigned to but never used

Remove assignment to unused variable sample_ids

(F841)

---

2039-2042: Simplify assignment with a ternary operator

The if-else block can be simplified using a ternary operator for better readability.

Apply this diff to simplify the code:

-    if curr_spaces:
-        spaces = ctx.spaces
-    else:
-        spaces = _parse_spaces(ctx, spaces)
+    spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)

🧰 Tools

🪛 Ruff (0.8.2)

2039-2042: Use ternary operator spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces) instead of if-else-block

Replace if-else-block with spaces = ctx.spaces if curr_spaces else _parse_spaces(ctx, spaces)

(SIM108)

---

2335-2335: Iterate directly over dictionary keys

When iterating over a dictionary, you can iterate directly over the keys without calling .keys().

Apply this diff to the code:

-        for path in schema.keys()
+        for path in schema

---

2354-2354: Iterate directly over dictionary keys

When iterating over a dictionary, you can iterate directly over the keys without calling .keys().

Apply this diff to the code:

-        for path in schema.keys()
+        for path in schema

🧰 Tools

🪛 Ruff (0.8.2)

2354-2354: Use key in dict instead of key in dict.keys()

Remove .keys()

(SIM118)

---

202-290: Reduce code duplication in input helper functions

The functions _clone_sample_field_inputs and _clone_frame_field_inputs have similar logic. Consider refactoring to extract common code into a shared helper function to improve maintainability and reduce duplication.

Also applies to: 321-418

fiftyone/plugins/core.py (1)

495-505: Consider enhancing error handling for built-in plugin loading.

While the implementation correctly handles both external and built-in plugins, consider adding specific error logging for built-in plugin failures to help with debugging.

 def _list_plugins(enabled=None, include_builtin=False):
     plugins = []
     external_plugin_paths = list(_iter_plugin_metadata_files())
     builtin_plugin_paths = list(
         _iter_plugin_metadata_files(BUILTIN_PLUGINS_DIR)
     )
+    if include_builtin and not builtin_plugin_paths:
+        logger.debug("No built-in plugins found in %s", BUILTIN_PLUGINS_DIR)
+
     all_plugin_paths = (
         (external_plugin_paths + builtin_plugin_paths)
         if include_builtin
         else external_plugin_paths
     )

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

plugins/operators/__init__.py (2)

2374-2406: Add docstring documentation to the register function

The new register() function should include docstring documentation explaining its purpose, parameters, and usage.

Add this docstring:

 def register(p):
+    """Register built-in operators with the plugin system.
+    
+    Args:
+        p: The plugin context to register operators with
+    """
     p.register(EditFieldInfo, builtin=True)

---

2374-2374: Add type hint for the register function parameter

Add a type hint to improve code clarity and enable better IDE support.

-def register(p):
+def register(p: "PluginContext") -> None:

Note: Add the following import at the top of the file:

from fiftyone.plugins.context import PluginContext

plugins/panels/fiftyone.yml (1)

6-6: Update the repository URL

The current URL points to a subdirectory in the main repository. Consider updating it to point to the actual plugin documentation or a more specific location.

-url: https://github.com/voxel51/fiftyone/plugins/panels
+url: https://github.com/voxel51/fiftyone/tree/main/plugins/panels

plugins/panels/__init__.py (2)

1-7: Enhance module documentation

The module docstring could be more descriptive about its purpose and functionality. Consider adding:

• Description of what built-in panels are
• How to register new built-in panels
• Example usage

 """
 Builtin panels.
+
+Built-in panels are core components of FiftyOne that provide essential
+functionality. This module handles the registration of built-in panels
+using the plugin system.
+
+Example:
+    To register a new built-in panel:
+
+    ```python
+    from fiftyone.plugins import get_plugin_context
+    
+    p = get_plugin_context()
+    p.register(MyPanel, builtin=True)
+    ```
 
 | Copyright 2017-2024, Voxel51, Inc.
 | `voxel51.com <https://voxel51.com/>`_
 |
 """

---

12-13: Add error handling and type hints to register function

The registration function could benefit from:

• Type hints for better code maintainability
• Error handling for registration failures
• Documentation string

-def register(p):
-    p.register(EvaluationPanel, builtin=True)
+from typing import Any
+from fiftyone.plugins.context import PluginContext
+
+def register(p: PluginContext) -> None:
+    """Register built-in panels with the plugin system.
+
+    Args:
+        p: The plugin context to register panels with
+
+    Raises:
+        ValueError: If registration fails
+    """
+    try:
+        p.register(EvaluationPanel, builtin=True)
+    except Exception as e:
+        raise ValueError(f"Failed to register built-in panel: {e}")

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Search for references to both names to ensure consistency
rg "model_evaluation_panel_builtin|EvaluationPanel" --type py

plugins/operators/__init__.py (1)

2374-2406: LGTM! Well-structured registration of built-in operators

The implementation successfully achieves the PR's objective of standardizing built-in plugin registration. The operators are logically grouped and consistently registered with the builtin=True flag.

Consider adding a docstring to the register function explaining its purpose and the significance of the builtin=True flag for future maintainers. This would help document the architectural decision to standardize built-in plugin registration.

Example:

def register(p):
    """Register built-in operators with the plugin system.
    
    This function implements the standardized method for registering built-in
    operators, ensuring consistency with the plugin architecture.
    
    Args:
        p: The plugin context to register operators with
    """

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

plugins/operators/__init__.py (2)

2373-2405: Add docstring documentation for the register function

The register function should include a docstring explaining its purpose and parameters.

Apply this diff to add documentation:

 def register(p):
+    """Register all built-in operators with the plugin system.
+
+    Args:
+        p: The plugin context to register operators with
+    """
     p.register(EditFieldInfo, builtin=True)
     p.register(CloneSelectedSamples, builtin=True)

---

2373-2405: Consider grouping operator registrations by functionality

The operator registrations could be organized into logical groups with comments to improve readability and maintainability.

Apply this diff to group the registrations:

 def register(p):
+    # Field operations
     p.register(EditFieldInfo, builtin=True)
     p.register(CloneSampleField, builtin=True)
     p.register(CloneFrameField, builtin=True)
     p.register(RenameSampleField, builtin=True)
     p.register(RenameFrameField, builtin=True)
     p.register(ClearSampleField, builtin=True)
     p.register(ClearFrameField, builtin=True)
     p.register(DeleteSampleField, builtin=True)
     p.register(DeleteFrameField, builtin=True)
 
+    # Sample operations
     p.register(CloneSelectedSamples, builtin=True)
     p.register(DeleteSelectedSamples, builtin=True)
     p.register(DeleteSelectedLabels, builtin=True)
 
+    # Index operations
     p.register(CreateIndex, builtin=True)
     p.register(DropIndex, builtin=True)
 
+    # Summary operations
     p.register(CreateSummaryField, builtin=True)
     p.register(UpdateSummaryField, builtin=True)
     p.register(DeleteSummaryField, builtin=True)
 
+    # Group operations
     p.register(AddGroupSlice, builtin=True)
     p.register(RenameGroupSlice, builtin=True)
     p.register(DeleteGroupSlice, builtin=True)
 
+    # View operations
     p.register(ListSavedViews, builtin=True)
     p.register(LoadSavedView, builtin=True)
     p.register(SaveView, builtin=True)
     p.register(EditSavedViewInfo, builtin=True)
     p.register(DeleteSavedView, builtin=True)
 
+    # Workspace operations
     p.register(ListWorkspaces, builtin=True)
     p.register(LoadWorkspace, builtin=True)
     p.register(SaveWorkspace, builtin=True)
     p.register(EditWorkspaceInfo, builtin=True)
     p.register(DeleteWorkspace, builtin=True)
 
+    # Utility operations
     p.register(SyncLastModifiedAt, builtin=True)
     p.register(ListFiles, builtin=True)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/unittests/operators/executor_tests.py (1)

34-34: Confirm the long-term plan for registering built-in operators.
Appending to the internal “_EXTRA_OPERATORS” list works for this test scenario; however, consider providing a dedicated method like "register_builtin" to maintain clarity and ensure consistent usage across the codebase.

fiftyone/operators/registry.py (1)

105-106: Clarify interplay between “include_builtin” and “builtins_only”.
When “builtins_only” is True, “include_builtin” becomes irrelevant. Consider making them mutually exclusive or documenting their intended interplay to avoid confusion.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

-        include_builtin=enabled != False,
+        include_builtin=bool(enabled),

plugins/operators/__init__.py (1)

2373-2405: Streamline operator registration.

The repeated register calls could be reduced by collecting all operator classes in a single list and iterating over them, making the code more concise and easier to maintain.

Suggested refactor:

 def register(p):
-    p.register(EditFieldInfo)
-    p.register(CloneSelectedSamples)
-    p.register(CloneSampleField)
-    ...
-    p.register(ListFiles)
+    operators = [
+        EditFieldInfo,
+        CloneSelectedSamples,
+        CloneSampleField,
+        CloneFrameField,
+        RenameSampleField,
+        ...,
+        ListFiles,
+    ]
+    for op in operators:
+        p.register(op)

fiftyone/plugins/definitions.py (1)

44-48: ⚠️ Potential issue

Add a missing return statement.

The builtin property currently does not return any value, which means it always evaluates to None. A return statement is needed to return the boolean result of directory matching.

Possible fix:

 @property
 def builtin(self):
     """Whether the plugin is a builtin plugin."""
-    self.directory.startswith(BUILTIN_PLUGINS_DIR)
+    return self.directory.startswith(BUILTIN_PLUGINS_DIR)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    @property
    def builtin(self):
        """Whether the plugin is a builtin plugin."""
        return self.directory.startswith(BUILTIN_PLUGINS_DIR)

plugins/operators/__init__.py (2)

2373-2405: Consider grouping operator registrations by functionality

The register function provides a good centralized location for operator registration, but it could be better organized. Consider grouping related operators together with comments indicating their purpose.

Apply this diff to improve organization:

 def register(p):
+    # Field operations
     p.register(EditFieldInfo)
     p.register(CloneSelectedSamples)
     p.register(CloneSampleField)
     p.register(CloneFrameField)
     p.register(RenameSampleField)
     p.register(RenameFrameField)
     p.register(ClearSampleField)
     p.register(ClearFrameField)
     p.register(DeleteSampleField)
     p.register(DeleteFrameField)
 
+    # Sample and label operations
     p.register(DeleteSelectedSamples)
     p.register(DeleteSelectedLabels)
 
+    # Index operations
     p.register(CreateIndex)
     p.register(DropIndex)
 
+    # Summary field operations
     p.register(CreateSummaryField)
     p.register(UpdateSummaryField)
     p.register(DeleteSummaryField)
 
+    # Group operations
     p.register(AddGroupSlice)
     p.register(RenameGroupSlice)
     p.register(DeleteGroupSlice)
 
+    # View operations
     p.register(ListSavedViews)
     p.register(LoadSavedView)
     p.register(SaveView)
     p.register(EditSavedViewInfo)
     p.register(DeleteSavedView)
 
+    # Workspace operations
     p.register(ListWorkspaces)
     p.register(LoadWorkspace)
     p.register(SaveWorkspace)
     p.register(EditWorkspaceInfo)
     p.register(DeleteWorkspace)
 
+    # Utility operations
     p.register(SyncLastModifiedAt)
     p.register(ListFiles)

---

Line range hint 1-2405: Consider splitting operators into separate modules

The current file is quite large and handles multiple types of operations. This can make maintenance, testing, and code navigation more challenging.

Consider reorganizing the code into separate modules:

1. Create a operators/ subdirectory
2. Split operators into logical groups in separate files:
   • field_operators.py for field-related operations
   • sample_operators.py for sample operations
   • view_operators.py for view operations
   • workspace_operators.py for workspace operations
   • etc.
3. Create a central registry.py that imports and registers all operators
4. Move utility functions to a separate utils.py module

This would improve:

• Code organization and maintainability
• Testing isolation
• Code navigation and readability
• Module cohesion

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro